Migrate trigger form params to new UI #45270

jscheffl · 2024-12-29T16:16:15Z

After @shubhamraj-git has added the trigger form to the new React UI, this PR now adds the flexible trigger form - Based on the logic of the former AIP-50 implementation - using DAG ParamsDict to render form fields based on field definitions.

Note that the form rendering was made as a component by intent as it will be re-used in Connection Forms in future to render connection extra fields.

To test the form rendering the following DAGs might be good candidates:

example_bash_decorator - has no params so should not display a specific form
example_bash_operator - has one basic parameter w/o any special thing. Simple as it can be
example_params_trigger_ui - has 4 parameters, still basic

... and with advanced section un-folded:
example_params_ui_tutorial - this is the most complex beast as it has all features as a demo that shall be supported.

...and the lower section un-folded:

Scope of the PR is the UI layout and logic to render. What is missing and will be a follow-up PR is the binding and sync of the form field content with the JSON dict that is used to trigger the DAG in the "Advanced Options" section.

shubhamraj-git · 2024-12-30T19:16:13Z

Thanks @jscheffl for working on this.
Had the first look. Looks great. The base is almost ready.

As mentioned in TODO, we will support the accordion and not flat.

Some points which i noticed, just writing it down incase we don't miss.

There is no live changes to conf json onBlur
Should have a validation for integer inputs.
Dropdown not working for select values
we should have a time picker.
Length check not working
~~6. JSON inputs should have a code view? (Just a opinion)~~
List should have multi lines

Overall this is going in right direction.

Should we have inbuilt scroll for the dynamic form? since the modal seems very long.

EDIT: Just saw a todo for point 6 (json code view) and most of the above issues as todo.

jscheffl · 2025-01-02T15:52:10Z

Thanks @jscheffl for working on this. Had the first look. Looks great. The base is almost ready.

As mentioned in TODO, we will support the accordion and not flat.

Some points which i noticed, just writing it down incase we don't miss.
1. There is no live changes to conf json onBlur
2. Should have a validation for integer inputs.
3. Dropdown not working for select values
4. we should have a time picker.
5. Length check not working
   ~6. JSON inputs should have a code view? (Just a opinion)~
6. List should have multi lines
Overall this is going in right direction.

Should we have inbuilt scroll for the dynamic form? since the modal seems very long.

EDIT: Just saw a todo for point 6 (json code view) and most of the above issues as todo.

(1) I recommend to make into a separate PR - as also discussed 1:1
(2,3,5,6) fixed.
(4) would need a component fix in react for Chrome... and a workaround for Firefox. Firfox time picking is "just broken" - would make this to a separate PR as well.

So... for the Layout part I'd say... ready for review!

shubhamraj-git

Thanks @jscheffl ! Overall it looks good. Just few comments.
When clicking on trigger run, the modal pops up with all the options at front.
Should we consider them in a accordion and if needed user can view?
Apart from it, had some general comments, which is similar for all files.

airflow/ui/src/components/FlexibleForm/FlexibleFormFieldAdvancedArray.tsx

airflow/ui/src/components/FlexibleForm/FlexibleFormFieldBool.tsx

airflow/ui/src/components/FlexibleForm/FlexibleFormFieldDate.tsx

shubhamraj-git

Found one glitch, which can be taken in further work.

Also, few things to add in list of TODO:

Reset button should reset everything on the Form, including the flexible form, and it should ask a validation pop up, since if by mistake user press it, it's lot to loose.
Accordion should be there for flexible form fields.
And of course connection between conf and flexible form.

airflow/ui/src/components/FlexibleForm/FlexibleFormFieldString.tsx

airflow/ui/src/components/FlexibleForm/FlexibleFormFieldObject.tsx

bbovenzi

I would argue against putting the form inside of an Accordion because it would hide required fields from the user.

2. Let's remove the `FlexibleForm` prefix from these components. It's too hard to read and is redundant with their parent folder.

jscheffl · 2025-01-11T19:58:30Z

Had a few "sleeps" over the discussion I had in Slack with @shubhamraj-git about Accordion.. and after Pierre also assigned it to the Note for Clear tasks... I am somewhat open and ajusted... as forms can get long. All is not (last commit) in one accordion group. Looks like this and parts are nicely folding:

Changing to Advanced options:

WDYT?

jscheffl · 2025-01-12T13:48:31Z

Hi @bbovenzi I aligned in Slack with @shubhamraj-git that he would take the "wire-up" of reset-button and sync of form fields <-> Advanced Options JSON in a follow up PR the next days. So this PR is mainly the general component start and layout, function coming in the next PR.

Would it be OK frmo point of code/layout/structure mo merge? Then would need your review/approval.

bbovenzi

Sorry this took me a bit to review.

Now I see how sections work with the accordions so ignore my previous comment.

It is a bit of a nitpick, but coudl we rename the field components? Along the lines of NumberInput instead of FieldNumber?

I'm excited we're finally able to migrate all of this to react! Great work!

airflow/ui/src/components/TriggerDag/TriggerDAGForm.tsx

airflow/ui/src/components/FlexibleForm/index.tsx

airflow/ui/src/components/FlexibleForm/FieldMultiSelect.tsx

airflow/ui/src/components/FlexibleForm/SelectElement.tsx

airflow/ui/src/queries/useDagParams.ts

airflow/ui/src/components/FlexibleForm/SelectElement.tsx

airflow/ui/src/components/FlexibleForm/index.tsx

airflow/ui/src/components/FlexibleForm/Hidden.tsx

airflow/ui/src/components/FlexibleForm/SelectElement.tsx

jscheffl · 2025-01-14T22:48:31Z

Sorry this took me a bit to review.

Now I see how sections work with the accordions so ignore my previous comment.

It is a bit of a nitpick, but coudl we rename the field components? Along the lines of NumberInput instead of FieldNumber?

I'm excited we're finally able to migrate all of this to react! Great work!

Uff, this was a very good review! Learned something new. I assume now it needs a second iteration review from the rework...
Had some challenges with the flex-basis - which is much cleaner in your proposal... but somehow not working. Any idea why?

bbovenzi

Found two things with the datetime field.

Also, did you see my comment on renaming the Fields and NormalRow?

airflow/ui/src/components/FlexibleForm/FieldDateTime.tsx

airflow/ui/src/components/FlexibleForm/FieldSelector.tsx

…o separate TSX file

jscheffl · 2025-01-15T23:11:46Z

Found two things with the datetime field.

Also, did you see my comment on renaming the Fields and NormalRow?

Next round of reviews applied.
Saw the request to rename "NormalRow" ... renamed it to proposed "FieldRow" - is this OK?
The other comment about renaming the Fields... I did not understand. Can you help me what you wanted me to rename to?

bbovenzi

Actually, this all looks pretty good. Let's get this merged and continue working on the rest

* Add flexible form fields to trigger form WIP * Add flexible form fields to trigger form WIP * Fix dropdown issue * Add support for arrays of strings * Add support for advanced arrays * Remove placeholder for string * Add support for proposals in string fields * Add support for multi-select dropdowns * Add support for object fields * Add support for number fields * Add support for multiline text fields * Use other component for multi-select which displays the values * Implement support for form sections * Implement support for form sections * Add an alert that form ist not fully implemented * Review feedback - safe string handling * Review feedback, remove reloading warnings, move selector functions to separate TSX file * Review feedback - rename components for shorter name * Review feedback - adjust hard-coded color for boder in CodeMirror * Rework default field values * Move form elements into a common accorion * Review feedback * Review feedback, next round * Remove un-needed placeholders in date-time picker

boring-cyborg bot added the area:UI Related to UI/UX. For Frontend Developers. label Dec 29, 2024

jscheffl added the AIP-50 Trigger DAG UI user Form label Dec 29, 2024

jscheffl force-pushed the feature/migrate-trigger-form-params-to-new-ui branch from 51d3c4d to 83c450e Compare December 29, 2024 22:21

jscheffl force-pushed the feature/migrate-trigger-form-params-to-new-ui branch 4 times, most recently from e7381e7 to 8067918 Compare January 2, 2025 15:41

jscheffl marked this pull request as ready for review January 2, 2025 15:52

jscheffl requested review from bbovenzi, pierrejeambrun and ryanahamilton as code owners January 2, 2025 15:52

shubhamraj-git reviewed Jan 3, 2025

View reviewed changes

jscheffl force-pushed the feature/migrate-trigger-form-params-to-new-ui branch from 8067918 to 313d868 Compare January 4, 2025 16:10

jscheffl requested a review from shubhamraj-git January 4, 2025 17:17

shubhamraj-git approved these changes Jan 5, 2025

View reviewed changes

airflow/ui/src/components/FlexibleForm/FlexibleFormFieldString.tsx Outdated Show resolved Hide resolved

bbovenzi reviewed Jan 7, 2025

View reviewed changes

airflow/ui/src/components/FlexibleForm/FlexibleFormFieldObject.tsx Outdated Show resolved Hide resolved

bbovenzi reviewed Jan 7, 2025

View reviewed changes

jscheffl force-pushed the feature/migrate-trigger-form-params-to-new-ui branch 2 times, most recently from 6b61cf4 to fcbe5d0 Compare January 9, 2025 21:06

jscheffl mentioned this pull request Jan 11, 2025

Configure HttpHook's auth_type from Connection #35591

Open

jscheffl force-pushed the feature/migrate-trigger-form-params-to-new-ui branch from fcbe5d0 to de1a47c Compare January 11, 2025 19:56

bbovenzi requested changes Jan 14, 2025

View reviewed changes

jscheffl force-pushed the feature/migrate-trigger-form-params-to-new-ui branch from de1a47c to 1c43ce6 Compare January 14, 2025 22:39

bbovenzi reviewed Jan 15, 2025

View reviewed changes

airflow/ui/src/components/FlexibleForm/FieldDateTime.tsx Outdated Show resolved Hide resolved

airflow/ui/src/components/FlexibleForm/FieldSelector.tsx Outdated Show resolved Hide resolved

Add flexible form fields to trigger form WIP

62d5c18

jscheffl added 22 commits January 15, 2025 23:35

Add flexible form fields to trigger form WIP

72c399b

Fix dropdown issue

1bb0df6

Add support for arrays of strings

afc50b2

Add support for advanced arrays

4cd9802

Remove placeholder for string

07f0ff1

Add support for proposals in string fields

b44534b

Add support for multi-select dropdowns

4bc3909

Add support for object fields

31fb834

Add support for number fields

6754033

Add support for multiline text fields

f9220ca

Use other component for multi-select which displays the values

b0b7cda

Implement support for form sections

507a7c6

Implement support for form sections

c589378

Add an alert that form ist not fully implemented

3a96f5a

Review feedback - safe string handling

0b4cc55

Review feedback, remove reloading warnings, move selector functions t…

030a067

…o separate TSX file

Review feedback - rename components for shorter name

9c626e3

Review feedback - adjust hard-coded color for boder in CodeMirror

2d18410

Rework default field values

907f11f

Move form elements into a common accorion

b27ebe9

Review feedback

d73aa62

Review feedback, next round

7028d76

jscheffl force-pushed the feature/migrate-trigger-form-params-to-new-ui branch from 1c43ce6 to 7028d76 Compare January 15, 2025 23:11

Remove un-needed placeholders in date-time picker

c7a96cf

jscheffl requested a review from bbovenzi January 16, 2025 06:43

bbovenzi approved these changes Jan 16, 2025

View reviewed changes

jscheffl merged commit 6d16136 into apache:main Jan 16, 2025
45 checks passed

shubhamraj-git mentioned this pull request Jan 19, 2025

Connect Trigger UI Conf with the flexible form #45790

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate trigger form params to new UI #45270

Migrate trigger form params to new UI #45270

jscheffl commented Dec 29, 2024 •

edited

Loading

shubhamraj-git commented Dec 30, 2024 •

edited

Loading

jscheffl commented Jan 2, 2025 •

edited

Loading

shubhamraj-git left a comment

shubhamraj-git left a comment

bbovenzi left a comment

jscheffl commented Jan 11, 2025

jscheffl commented Jan 12, 2025

bbovenzi left a comment

jscheffl commented Jan 14, 2025

bbovenzi left a comment

jscheffl commented Jan 15, 2025

bbovenzi left a comment

Migrate trigger form params to new UI #45270

Migrate trigger form params to new UI #45270

Conversation

jscheffl commented Dec 29, 2024 • edited Loading

shubhamraj-git commented Dec 30, 2024 • edited Loading

jscheffl commented Jan 2, 2025 • edited Loading

shubhamraj-git left a comment

Choose a reason for hiding this comment

shubhamraj-git left a comment

Choose a reason for hiding this comment

bbovenzi left a comment

Choose a reason for hiding this comment

jscheffl commented Jan 11, 2025

jscheffl commented Jan 12, 2025

bbovenzi left a comment

Choose a reason for hiding this comment

jscheffl commented Jan 14, 2025

bbovenzi left a comment

Choose a reason for hiding this comment

jscheffl commented Jan 15, 2025

bbovenzi left a comment

Choose a reason for hiding this comment

jscheffl commented Dec 29, 2024 •

edited

Loading

shubhamraj-git commented Dec 30, 2024 •

edited

Loading

jscheffl commented Jan 2, 2025 •

edited

Loading